fix(cli): persist what the server answered for a captured page - #3553
Conversation
A page that renders is not a page that succeeded. An error page has a title, a palette, typefaces and a DOM, so every extractor downstream reads it happily and produces a design system belonging to whoever wrote the error page rather than to the site's owner. The status was already read, once, to feed `detectBlockedPage`, and then dropped. That helper cannot stand in for it: it decides whether the rendered document LOOKS like a protection wall, over a minimal-DOM heuristic that a rich error page passes. "Was this blocked?" and "what did the server answer?" are two questions, and widening the first to carry the second would leave a heuristic owning a fact. So the response status is persisted plainly, as its own record, and every consumer decides for itself what a non-success response means for its product. Written before the blocked-page check runs, so the record's absence means "navigation never produced a response" — a third state distinct from a status of 404 and from a status of null.
…tput The status reached `CaptureResult` and was then dropped at the CLI boundary, which is the same read-once-and-discard that made the error page harvestable in the first place. `--json` is the documented programmatic surface, and an agent reading `ok: true` off a capture of a 404 has no way to see it there.
jerrai-bot-heygen
left a comment
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed at c3f279d299e299170882fc0a40bda07379caf216.
response.json, the in-process result, and the JSON CLI output preserve the same final navigation status without conflating null, 0, and a response status. The record is written before the independent blocked-page heuristic, so callers retain the server fact even when the capture produces a rich non-success document. Focused CLI/type/lint checks are green and no inline threads are open.
— Jerrai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at head c3f279d299e299170882fc0a40bda07379caf216. Additive to @jerrai-bot-heygen's approve; this is a human pass over the whole diff, since it was the only review on the PR.
Strengths, and they are the reason this reads well:
- The "Why not widen
detectBlockedPage" section is the right call andresponseRecord.ts:5-12carries the reasoning where the next reader will find it. "Was this blocked?" and "what did the server answer?" genuinely are different questions, and making the heuristic own the second would leave a fact gated on a guess that a rich error page defeats anyway. - The three-state design is real, not decoration, and the ordering is what makes it work. The write at
index.ts:303lands before the blocked-page check at:305, so absence means "navigation produced no response" - andextracted/is created at:121withrecursive: true, well before navigation at:250, so the write cannot fail for a missing directory on any path that reaches it. I checked that specifically because a writer documented as needing "an already-created directory" is where this design would break. - The test is non-vacuous in the way that matters:
.not.toBe(0)and"status" in recordpin the falsy-coercion trap, which is exactly how anumber | nullfield silently degrades into "0 means we do not know."
important - base drift, and the PR's own verification is what it costs. This head is 43 commits behind main, and all three files it modifies have changed there since the merge base 0fd70b1d. Most relevant: main has already added a required field to this same interface - dropped: AssetDropCounts (capture/types.ts) - so after a merge CaptureResult requires both dropped and httpStatus.
I traced whether that collides, rather than just flagging it, and it composes cleanly in all three places: main inserts dropped after assets while this PR inserts httpStatus after url, in the interface, in the return literal (index.ts:884 here vs :919 on main), and in the --json block (commands/capture.ts). Non-overlapping insertions, so both fields survive and the merged construction site satisfies both. That is the same design pattern applied to a second discarded fact, which is why it lands in the same three spots and still fits.
What is actually stale is the verification, not the code: typecheck, the 2867-test run, and the real-Chrome checks were all performed against a base with no dropped. Rebasing and re-running is the cheap way to make the PR's own claims describe the state that would land.
nit - the writer is unit-tested, but nothing asserts the value reaches capture --json, which is the surface the rationale is about ("an agent reading ok: true off a capture of a 404 could not see it"). It stays a nit because the wiring is a two-line pass-through and typecheck guarantees the field exists; an assertion that the emitted JSON carries httpStatus would close the gap for the price of one line.
Three things I checked that are NOT findings, recorded so the next reviewer does not repeat the sweep:
CaptureResultappears in nine files across three packages, which looks like a required-field audit problem and is not one.packages/engine/src/types.ts:202declares its ownCaptureResultandproducerimports that one - same name, unrelated type.navigateForCapture.ts:18isNavigateForCaptureResult, a substring match.scaffolding.ts:56usesCaptureResult["animationCatalog"], an indexed access unaffected by new fields. The cli interface has exactly one construction site, and it sets the field.- The unwrapped
writeFileSyncat:303matches this file's own mixed convention -:402and:425are bare,:435and:717are wrapped - so it is consistent rather than an omission, and the directory-existence risk is already ruled out above. httpStatusis passed todetectBlockedPagefrom the same local the record is written from, so the body's "same value" claim holds by construction rather than by coincidence.
Trusting (not re-run): the 2867-test and typecheck results, and the real-Chrome 404-vs-200 capture.
Verdict: APPROVE
Reasoning: The design separates a fact from a heuristic instead of overloading the heuristic, the three-state semantics are backed by the write ordering rather than asserted, and the one required-field collision with main composes cleanly. The base drift costs the PR its verification, not its correctness.
- Rames Jusso
4 participants
The failure
hyperframes captureagainst a URL that answers 404 completes successfully. The error page has a title, a palette, typefaces, sections and a DOM, so every extractor downstream reads it happily, and the capture returnsok: truewith a full design token set. Nothing in the output says the server refused.Reproduced against a local server that answers 404 with a styled error page — 11 colours and 4 typefaces extracted,
lastPhase: complete:The status was already read, once, to feed
detectBlockedPage, and then dropped on the floor. A consumer of a capture directory therefore had no way to learn it.Why not widen
detectBlockedPageAdding 404 to its status set looks like a one-line fix and is the wrong one. That helper answers "does the rendered document look like an access-protection wall?" — a heuristic gated on a minimal DOM, which a rich error page fails anyway. "Was this blocked?" and "what did the server answer?" are two different questions, and making the heuristic the carrier for the second leaves a fact owned by a guess.
The change
The status is persisted plainly, as its own record:
extracted/response.json→{ "status": 404 }, written from the same valuedetectBlockedPagealready receives.CaptureResult.httpStatus, so an in-process caller does not have to read a file the function just wrote.httpStatusincapture --json, the documented programmatic surface. Leaving it out would repeat the same read-once-and-discard one boundary later: an agent readingok: trueoff a capture of a 404 could not see it.Written before the blocked-page check, so the record's absence means "navigation never produced a response" — a third state, distinct from a status of
404and from a status ofnull. Those three are not collapsed:nullis "we never learned what the server said", which is not "fine".No behaviour changes here. Deciding what a non-success response means is left to each consumer, which is why this ships as a fact rather than a refusal.
Verification
bun run --cwd packages/cli test— 197 files, 2867 passed, 3 skipped, 0 failed.bun run --cwd packages/cli typecheck— clean (afterpackages/corebuild).oxlint/oxfmt --checkon the changed files — clean.{"status": 404}; the same page served as 200 persists{"status": 200}.nullto0in the writer fails it withexpected +0 to be null; restored byte-exact and it passes again.